-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add events page #66
base: dev
Are you sure you want to change the base?
Conversation
The ARIA label is placed correctly, however lighthouse can't find which text it's referring to
Event card skeleton is slightly shorter than event card. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at UI next time, but skimmed through the code now and commented on what I remembered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI things:
- Badge needs either tooltip or pointer-events-none. since it has a hover effect.
- Add a larger padding between the text and the Image.
- Add a hover effect to the Cards.
- Fix the ring effect for the Link element used on the cards so it works with keyboard navigation.
- The suspense cards are a little shorter than the actual cards.
- As an addition it would maybe be nice to make the heading clickable so it scrolls down to them. I think this can be done with a simple id and a link or something using #.
- Style the text for the time differently from the rest of the cards. I dont know the best way, maybe put it in a strong tag to make it bold or look at examples online
<> | ||
<h1 className='my-4'>{t('title')}</h1> | ||
<Link href='#active'> | ||
<h2 className='my-2' id='active'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<h2 className='my-2' id='active'> | |
<h2 className='pt-4 mb-4' id='active'> |
Should have consistent spacing
))} | ||
</div> | ||
<Link href='#past'> | ||
<h2 className='my-4' id='past'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<h2 className='my-4' id='past'> | |
<h2 className='pt-4 mb-4' id='past'> |
mt-4 and my-4 doesn't add space to the top for some reason, pt-4 mb-4 will give the same styling as in loading.tsx
return ( | ||
<> | ||
<h1 className='my-4'>{t('title')}</h1> | ||
<h2 className='my-2'>{t('activeEvents')}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<h2 className='my-2'>{t('activeEvents')}</h2> | |
<h2 className='my-4'>{t('activeEvents')}</h2> |
Consistent styling with other h2 elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is still ~5px shorter than the actual component.
export default function EventDetailsLoading() { | ||
return ( | ||
<> | ||
<Skeleton className='my-4 h-12 w-3/4 rounded-lg' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Skeleton className='my-4 h-12 w-3/4 rounded-lg' /> | |
<Skeleton className='mt-6 mb-4 h-12 w-3/4 rounded-lg' /> |
To keep same layout as in the actual page
const t = await getTranslations('events'); | ||
return ( | ||
<> | ||
<Button variant='secondary' aria-label='Back to Events' asChild> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aria-label isn't translated
The page lists events, separated into active, upcoming and past events.
There's also a separate page each event, so more information can be shown.